Fix #10129: Fix comparisons of applied type aliases #10221

odersky · 2020-11-07T13:22:34Z

Given

type F[Xs] = ...
F[As] <: F[Bs]

Do we dealias F or check the arguments directly? It turns out that neither choice
is correct in all instances. Checking the arguments directly is necessary for hk
type inference, but may narrow the constraint too much. The solution is to use
an either that tries both alternatives.

smarter · 2020-11-07T15:00:15Z

compiler/src/dotty/tools/dotc/core/TypeComparer.scala

+          // This leads to the constraint Lifted[U] <: Lifted[Int]. If we just
+          // check the arguments this gives `U <: Int`. But this is wrong. Dealiasing
+          // `Lifted` gives `Err | U <: Err | Int`, hence it should be `U <: Err | Int`.


I'm not necessarily against changing it now if we believe it's worth it, but that was actually an intentional behavior, see the justification in 5fb13aa:

I think the new behavior is arguably better since using type aliases now
gives you more control on how type inference proceeds, even if it means
that some things that used to typecheck don't anymore.

I think we should not simply infer something that does not follow from the constraint. Or at least we should do it under the strict control of either. i10129 is arguably a very natural way to express things.

Or, let's put it that way. The correct thing to do is to dealias. Unfortunately that breaks hk type inference. But hk type inference always was an ad-hoc best effort thing (some might call it a kludge). It's clear we can't do hk type inference in general. So it's good to push this as far as we can, but it should not compromise the correct behavior for other constraints.

odersky · 2020-11-07T17:55:37Z

test performance please

dottybot · 2020-11-07T17:56:37Z

performance test scheduled: 1 job(s) in queue, 0 running.

dottybot · 2020-11-07T19:14:20Z

Performance test finished successfully:

Visit http://dotty-bench.epfl.ch/10221/ to see the changes.

Benchmarks is based on merging with master (5c84e42)

smarter · 2020-11-08T16:42:27Z

compiler/src/dotty/tools/dotc/core/TypeComparer.scala

+                        // that is taken care of by the outer inFrozenGadtIf test. The problem is
+                        // that hk-alias-unification.scala relies on the right isSubArgs being performed
+                        // when constraining the result type of a method. But there we are only allowed
+                        // to use a necessaryEither, since it might be that an implicit conversion will


There's no implicit conversion in hk-alias-unification.scala, just implicit search.

You are right. The change was actually introduced here: #8635

It was a way to avoid overcommitting when constraining the result type. Unfortunately, it seems that this causes the problems we encounter here. So, what to do? All this is getting more and more ad-hoc, which is not very satisfactory.

smarter · 2020-11-08T16:43:02Z

compiler/src/dotty/tools/dotc/core/TypeComparer.scala

+                        // be inserted on the result. So strictly speaking by going to a sufficientEither
+                        // we overshoot and narrow the constraint unnecessarily. On the other hand, this


In situation where the gadt constraints aren't frozen, isn't this narrowing problematic? /cc @abgruszecki

I believe not, since we are in a frozen Gadt state anyway.

Ah, we're in inFrozenGadtIf(!tyconIsInjective), and tyconIsInjective won't be true if both constructors are type aliases, so gadt constraints are frozen indeed, although it's not immediately obvious from the code.

smarter · 2020-11-08T16:44:32Z

compiler/src/dotty/tools/dotc/core/TypeComparer.scala

+                        // which demands a sufficientEither anyway.
+                        sufficientEither(
+                          isSubArgs(args1, args2, tp1, tparams),
+                          recur(tp1.superType, tp2.superType))


We check above that tycon1sym.isAliasType so tp1.superType should be equivalent to tp1, on the other hand I don't see a similar check for tp2, so it seems that tp2.superType could in fact be an actual supertype of tp2.

The code is only hit if the two type constructors are the same.

smarter · 2020-11-08T17:08:35Z

compiler/src/dotty/tools/dotc/core/TypeComparer.scala

+                          case tp1a as AppliedType(_, args1a) if args1a.eqElements(args1) =>
+                            tp2.superType match
+                              case tp2a as AppliedType(_, args2a) if args2a.eqElements(args2) =>
+                                return recur(tp1a, tp2a)


This branch is never taken (I added an assert here and verified that our whole CI passes with it), because we eagerly dealias in appliedTo unless it affects type inference: https://github.com/lampepfl/dotty/blob/8e9ac7519b56a452348b878979f56ff01308cc8e/compiler/src/dotty/tools/dotc/core/TypeApplications.scala#L319-L325

Ah, good to know!

smarter · 2020-11-09T14:34:20Z

compiler/src/dotty/tools/dotc/core/TypeComparer.scala

+                        // There's one test in shapeless/deriving that had to be disabled
+                        // (modules/deriving/src/test/scala/shapeless3/deriving/deriving.scala, value v7 in the functor test).


FYI @milessabin, how important is that particular test?

That's an important test. It demonstrates the ability to derive functor instances for nested type constructors.

It would at least be good to know why v7 fails whereas v6 succeeds. Both involve recursion. The only difference is that the former failing one has an additional non-recursive wrapper, which is itself successfully derived in v3.

odersky · 2020-11-10T07:20:27Z

That's the second time I get a conflict because of shapeless which causes me a huge effort to re-sync. I'll do that one last time and then merge since I don't have the time to go through these steps a third time.

milessabin · 2020-11-10T10:37:10Z

The failure with shapeless here is demonstrating what this change is forcing us to give up.

That being the case, it would help to have a motivation for the change and a sketch of a workaround.

odersky · 2020-11-10T11:07:00Z

The motivation is issue #10129.

Here we have

type Lifted[A] = Err | A

and the constraint

Lifted[X] <: Lifted[Int]

It's simply incorrect to conclude from this that X <: Int. So that means in general we have to dealias before comparing. But doing this breaks some higher-kinded type inference moves. So the compromise in this PR is to look at both the direct arguments and the dealiased type and pick the solution that constrains less. That still allows hk type inference to work, unless the dealiased
comparison would constrain things strictly less.

I don't know what else one could do here.

milessabin · 2020-11-10T11:23:49Z

That sounds completely reasonable.

What's not clear to me is why that breaks the test in shapeless. Other than involving hk types it doesn't appear to be related in any obvious way to the example you're fixing here.

You suggest this is an inference problem ... is there an explicit type annotation or parameter which could be added to the test which would fix it? What is the compilation error in shapeless for that test?

odersky · 2020-11-10T17:22:02Z

What's not clear to me is why that breaks the test in shapeless. Other than involving hk types it doesn't appear to be related in any obvious way to the example you're fixing here.

It's hard to diagnose since it is in auto-generated code from deriving, I believe. What must have happened is that there is a type alias somewhere such as

    type T[X] = Any

and a constraint gets generated such as

    T[F[A]]  <: T[List{X]]

In this case we'd conclude so far that F = List. But after dealiasing it's just Any <: Any which does not constrain at all. So we do not instantiate F.

The RHS of Any is just for illustration, it could be anything that is already a supertype of the lhs T[F[A]].

milessabin · 2020-11-10T18:12:34Z

There are uses of a Const type constructor in shapeles, ie.

type Const[c] = [t] =>> c

which could produce something similar to your type T[X] = Any (with something other than Any on the RHS).

Are you saying that this sort of construct can't be relied on any more? Can you suggest an alternative encoding?

odersky · 2020-11-10T18:15:05Z

Yes, Const would be problematic for the same reasons as my example.Not sure what to do. I imagine Const can't be opaque. right?

smarter · 2020-11-10T18:24:36Z

I think this comes down to a design decision: do we want type aliases to be a way to drive type inference, or do we want to always infer the minimum set of constraints? The former let library authors craft aliases to improve inference in their APIs and hopefully provide a better UX, but it's ill-defined, whereas the latter can be unintuitive for users but it's a clear criterion compiler developers can refer to when making changes to type inference, so it reduces the risk that we end up piling up heuristics after heuristics without really knowing what we're doing.
I don't know what the correct answer is here, but given that using type aliases to drive inference is already well-established with HK unification which works fairly well in practice, I don't really mind that Lifted[?U] <: Lifted[A] infers ?U <: A (or something else depending on variance) no matter what Lifted is.

milessabin · 2020-11-10T18:25:04Z

I imagine Const can't be opaque. right?

No, it can't.

smarter · 2020-11-10T18:32:07Z

A trick might be to write type Const[c] >: [t] =>> c <: [t] =>> c so the compiler isn't so eager to get rid of the alias (I think we wouldn't end up triggering the code added in this PR because isAliasType will return false), but even if it works it's not a very nice solution.

odersky · 2020-11-10T19:41:35Z

Const[c] >: [t] =>> c <: [t] =>> c

Yes, that looks promising.

odersky · 2020-11-12T08:48:07Z

How should we proceed? We don't seem to have an alternative to the fix in this PR. I propose we get it in and then see what needs to be done to bring back the failing test in shapeless.

milessabin · 2020-11-12T10:28:05Z

I'm still unclear why this test fails but others don't.

Const is used pervasively in shapeless, and there's a presumption in the Dotty type class derivation infrastructure that something like Const be workable. If this is an edge case with a workaround it's not a big deal, but then it would help to understand why this is an edge case whereas other uses of Const aren't. If it isn't then there's a chance that this will damage type class derivation across the board.

- streamline comparisons of applied types - optimize sufficientEither to not try the second alternative if the first succeeds without narrowing the constraint.

Given type F[Xs] = ... F[As] <: F[Bs] Do we dealias `F` or check the arguments directly? It turns out that neither choice is correct in all instances. Checking the arguments directly is necessary for hk type inference, but may narrow the constraint too much. The solution is to go to an either that tries both options.

odersky · 2020-11-12T20:50:25Z

Good news! I found a way to do what we want and still keep all shapeless tests.

Digging deeper I found that the issue was not what I initially suspected. I suspected that a constraint generated by a isSubArgs was rolled back by the test of the dealiased types. But what happened instead was that we had a failing isSubArgs, and the alternative that recurred on the dealiased types somehow missed something which then led to this error:

-- Error: deriving.scala:104:43 ------------------------------------------------
104 |    val v7 = Functor[[t] =>> CList[Opt[t]]]
    |                                           ^
    |ambiguous implicit arguments: both method functorGen in object Functor and method given_Functor_F in object Functor match type shapeless3.deriving.Functor[
    |  [t] =>> shapeless3.deriving.adts.CList[shapeless3.deriving.adts.Opt[t]]
    |] of parameter ff of method apply in object Functor
1 error found

If we simply fail isMatchingApply on a failed isSubArgs then the test compiles. So this means that the different paths we took for checking the dealiased types somehow made a difference. In the failing example we dealised both types in lockstep whereas in the original code we dealiased first the supertype and then went through a second check via compareApplied1. Presumably that second check could have done some hk inference that we missed by dealiasing both types at the same time.

The fix is simple: Fail if isSubArgs fails. If it succeeds, also check the dealiased types and pick the weaker constraint.

milessabin · 2020-11-13T10:59:03Z

Good news! I found a way to do what we want and still keep all shapeless tests.

Awesome ... thanks for digging in to this! 😄

odersky marked this pull request as draft November 7, 2020 13:22

odersky force-pushed the fix-#10129 branch from 6caab70 to 7b35136 Compare November 7, 2020 14:25

odersky changed the title ~~Fix #10129: Fix comparisons of applied types~~ Fix #10129: Fix comparisons of applied type aliases Nov 7, 2020

odersky requested review from smarter and abgruszecki November 7, 2020 14:53

odersky assigned smarter Nov 7, 2020

odersky closed this Nov 7, 2020

odersky reopened this Nov 7, 2020

odersky marked this pull request as ready for review November 7, 2020 14:54

smarter reviewed Nov 7, 2020

View reviewed changes

odersky force-pushed the fix-#10129 branch from 4d3cbb2 to 5c7c5a2 Compare November 7, 2020 17:54

smarter requested changes Nov 8, 2020

View reviewed changes

odersky force-pushed the fix-#10129 branch from 89380a2 to 1303c84 Compare November 8, 2020 22:48

smarter reviewed Nov 9, 2020

View reviewed changes

odersky force-pushed the fix-#10129 branch from 1303c84 to 8d29f78 Compare November 9, 2020 19:57

odersky force-pushed the fix-#10129 branch from 8d29f78 to 35c1ca4 Compare November 10, 2020 07:25

odersky added 3 commits November 12, 2020 18:05

Refactor some TypeComparer code

6033fce

- streamline comparisons of applied types - optimize sufficientEither to not try the second alternative if the first succeeds without narrowing the constraint.

Drop dead code, and improve explanation

965efd9

odersky force-pushed the fix-#10129 branch from 35c1ca4 to 90cc0be Compare November 12, 2020 18:55

Try another variant

88dbba4

odersky force-pushed the fix-#10129 branch from 90cc0be to 88dbba4 Compare November 12, 2020 20:41

odersky merged commit 93f24a0 into scala:master Nov 14, 2020

odersky deleted the fix-#10129 branch November 14, 2020 18:06

smarter mentioned this pull request Mar 23, 2021

Type inference issue involve type alias and higher-kinded type #11499

Closed

Kordyjan added this to the 3.0.0 milestone Aug 2, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix #10129: Fix comparisons of applied type aliases #10221

Fix #10129: Fix comparisons of applied type aliases #10221

odersky commented Nov 7, 2020 •

edited

Loading

smarter Nov 7, 2020

odersky Nov 7, 2020

odersky Nov 7, 2020

odersky commented Nov 7, 2020

dottybot commented Nov 7, 2020

dottybot commented Nov 7, 2020

smarter Nov 8, 2020

odersky Nov 8, 2020

smarter Nov 8, 2020

odersky Nov 8, 2020

smarter Nov 8, 2020

smarter Nov 8, 2020

odersky Nov 8, 2020

smarter Nov 8, 2020

odersky Nov 8, 2020

smarter Nov 9, 2020

milessabin Nov 10, 2020

odersky commented Nov 10, 2020

milessabin commented Nov 10, 2020

odersky commented Nov 10, 2020

milessabin commented Nov 10, 2020 •

edited

Loading

odersky commented Nov 10, 2020 •

edited

Loading

milessabin commented Nov 10, 2020

odersky commented Nov 10, 2020

smarter commented Nov 10, 2020 •

edited

Loading

milessabin commented Nov 10, 2020

smarter commented Nov 10, 2020

odersky commented Nov 10, 2020

odersky commented Nov 12, 2020

milessabin commented Nov 12, 2020

odersky commented Nov 12, 2020 •

edited

Loading

milessabin commented Nov 13, 2020

		// be inserted on the result. So strictly speaking by going to a sufficientEither
		// we overshoot and narrow the constraint unnecessarily. On the other hand, this

		// There's one test in shapeless/deriving that had to be disabled
		// (modules/deriving/src/test/scala/shapeless3/deriving/deriving.scala, value v7 in the functor test).

Fix #10129: Fix comparisons of applied type aliases #10221

Fix #10129: Fix comparisons of applied type aliases #10221

Conversation

odersky commented Nov 7, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

odersky commented Nov 7, 2020

dottybot commented Nov 7, 2020

dottybot commented Nov 7, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

odersky commented Nov 10, 2020

milessabin commented Nov 10, 2020

odersky commented Nov 10, 2020

milessabin commented Nov 10, 2020 • edited Loading

odersky commented Nov 10, 2020 • edited Loading

milessabin commented Nov 10, 2020

odersky commented Nov 10, 2020

smarter commented Nov 10, 2020 • edited Loading

milessabin commented Nov 10, 2020

smarter commented Nov 10, 2020

odersky commented Nov 10, 2020

odersky commented Nov 12, 2020

milessabin commented Nov 12, 2020

odersky commented Nov 12, 2020 • edited Loading

milessabin commented Nov 13, 2020

odersky commented Nov 7, 2020 •

edited

Loading

milessabin commented Nov 10, 2020 •

edited

Loading

odersky commented Nov 10, 2020 •

edited

Loading

smarter commented Nov 10, 2020 •

edited

Loading

odersky commented Nov 12, 2020 •

edited

Loading